fix(engine-core): dedupe additions to WeakMultimap @W-21387024#5738
fix(engine-core): dedupe additions to WeakMultimap @W-21387024#5738
WeakMultimap @W-21387024#5738Conversation
Also remove legacy compat code
| * It leaks in legacy browsers, which may be undesired. | ||
| * | ||
| * This implementation relies on the WeakRef/FinalizationRegistry proposal. | ||
| * For some background, see: https://github.com/tc39/proposal-weakrefs |
There was a problem hiding this comment.
Since this is a stage 4, should be safe to point to mdn ?
| // We could check for duplicate values here, but it doesn't seem worth it. | ||
| // We transform the output into a Set anyway | ||
| // Skip adding if already present | ||
| for (const weakRef of weakRefs) { |
There was a problem hiding this comment.
Should we update the contract to: Set<WeakRef<V>>(), since it looks like that's what we want.
There was a problem hiding this comment.
It's kind of confusing because and the get method returns Set<V>, and we're comparing V but storing a group of WeakRef<V>. The values in the _map are purely internal, so it doesn't make a huge difference whether it's a set or an array.
According to this article, addition and iteration are comparable for sets and arrays, but sets have faster deletion. I made the switch.
rax-it
left a comment
rax-it
left a comment
There was a problem hiding this comment.
I also think we should do CLCO cutoff before changing this, could be problematic 🤷🏼♂️
* fix(engine-core): dedupe additions to `WeakMultimap` Also remove legacy compat code * chore: update link and fix case * chore: remove legacy note
2 participants
Skipping adding to the array for refs already present improves performance.
Because
WeakRefis baseline widely available, I've also removed the fallback class definition.Details
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
GUS work item